Add custom authenticator #24
Conversation
vustef
left a comment
vustef
left a comment
There was a problem hiding this comment.
Thanks Gerald, I like how this turned out. I just have one minor concern regarding exposing this in RestCatalogBuilder vs RestCatalog
crates/catalog/rest/src/catalog.rs
Outdated
| /// Set a custom token authenticator. | ||
| /// | ||
| /// The authenticator will be used to obtain tokens instead of using static tokens | ||
| /// or OAuth credentials. This must be called before any catalog operations. |
There was a problem hiding this comment.
why not add this method to the RestCatalogBuilder instead? That way by the time catalog is created, one either provided the token authenticator or not. And maybe we get rid of OnceCell.
crates/catalog/rest/src/client.rs
Outdated
| req.headers_mut().insert( | ||
| http::header::AUTHORIZATION, | ||
| format!("Bearer {token}").parse().map_err(|e| { | ||
| Error::new(ErrorKind::DataInvalid, "Invalid token from authenticator!") | ||
| .with_source(e) | ||
| })?, | ||
| ); |
There was a problem hiding this comment.
This code is exactly the same as the one below at line 266, except for the error message. Can we avoid duplication? Ideally through control flow, but if not, then by having this as a function
| // Verify authenticator was called | ||
| let count = *call_count.lock().unwrap(); | ||
| assert!( | ||
| count > 0, |
There was a problem hiding this comment.
should we check for exact count? so that not only we verify it was called once, but that it was called multiple times, and therefore refresh will work too.
There was a problem hiding this comment.
I see actually that's what the next test does. Any reason to have these two as separate tests?
vustef
left a comment
vustef
left a comment
There was a problem hiding this comment.
Just a few more, mostly test harness cleanup. But LGTM otherwise
crates/catalog/rest/src/client.rs
Outdated
| format!("Bearer {token}").parse().map_err(|e| { | ||
| Error::new( | ||
| ErrorKind::DataInvalid, | ||
| "Invalid token received from catalog server!", |
There was a problem hiding this comment.
We should customize the message, as the token has different source depending what is the place of invocation of this function
| async fn test_catalog_with_custom_token_authenticator() { | ||
| let catalog = get_catalog().await; | ||
| async fn test_authenticator_token_refresh() { | ||
| set_up(); |
There was a problem hiding this comment.
Instead of duplicating this code, let's reuse get_catalog. Maybe create get_catalog_with_authenticator, that wraps get_catalog. Or have private method that both call.
There was a problem hiding this comment.
Or some other solution, but we should hide all these details like get_catalog does.
There was a problem hiding this comment.
Added a custom get_catalog method
There was a problem hiding this comment.
I tried to convey that it should reuse code with fn get_catalog though, would that be possible?
2 participants
Implements a custom authenticator for
HttpClientandRestCatalog.